-
-
Notifications
You must be signed in to change notification settings - Fork 254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add @maven//:outdated to check for updated versions of artifacts #465
Conversation
|
||
set -euo pipefail | ||
|
||
if [ -f "private/tools/prebuilt/outdated_deploy.jar" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if there is a better way to check if the action is running inside the rules_jvm_external repo or outside of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an open issue that coursier should run with the java runtime configured for the build (#445) I'd suggest relying on that, and then you could use $(location)
to expand the location of the deploy jar in a rule or macro that writes this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell #445 is still an open issue because JAVABASE is not exposed to repository rules. If we fix JAVABASE for the java that coursier uses we should fix it here also but I don't currently see a straightforward fix otherwise.
releaseVersion = getReleaseVersion(repository, groupId, artifactId); | ||
if (releaseVersion != null) { | ||
verboseLog(String.format("Found version [%s] for %s:%s in %s", releaseVersion, groupId, artifactId, repository)); | ||
// Should we search all repositories in the list for latest version instead of just the first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the first one found has been working for me but may not find the truly latest version of an artifact in the list of repositories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is reasonable, but to reduce potential confusion, I think the repository where this lookup succeeded should be printed regardless of RJE_VERBOSE
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind if we leave this only in verbose mode? Mainly because it keeps the output clean and I'm not sure I know of any other outdated tool that tells you where it found the update by default.
40f94de
to
edec3d4
Compare
edec3d4
to
e36805e
Compare
e36805e
to
f0167ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic in Outdated.java
looks good to me.
|
||
set -euo pipefail | ||
|
||
if [ -f "private/tools/prebuilt/outdated_deploy.jar" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an open issue that coursier should run with the java runtime configured for the build (#445) I'd suggest relying on that, and then you could use $(location)
to expand the location of the deploy jar in a rule or macro that writes this file.
List<String> artifacts = Files.readAllLines(Paths.get(artifactsFilePath), StandardCharsets.UTF_8); | ||
List<String> repositories = Files.readAllLines(Paths.get(repositoriesFilePath), StandardCharsets.UTF_8); | ||
|
||
System.out.println(String.format("Checking for updates of %d artifacts against the following repositories:", artifacts.size())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: System.out.printf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mainly kept it with String.format
since it matches all of the other logging in the file that uses verboseLog
instaed of System.out.println
f0167ca
to
6b9d9c6
Compare
artifacts = [] | ||
for a in repository_ctx.attr.artifacts: | ||
artifacts.append(json_parse(a)) | ||
for artifact in repository_ctx.attr.artifacts: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, please separate refactoring changes from feature changes for a smaller diff :)
releaseVersion = getReleaseVersion(repository, groupId, artifactId); | ||
if (releaseVersion != null) { | ||
verboseLog(String.format("Found version [%s] for %s:%s in %s", releaseVersion, groupId, artifactId, repository)); | ||
// Should we search all repositories in the list for latest version instead of just the first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is reasonable, but to reduce potential confusion, I think the repository where this lookup succeeded should be printed regardless of RJE_VERBOSE
.
6b9d9c6
to
0f12829
Compare
This adds an outdated action to a maven_install repository so you can run
to check for updated versions of artifacts. e.g.